Fix validation logic for dataset upload form#422
Conversation
Deploying geodatalytics with
|
| Latest commit: |
9542e69
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8a52f1ef.geodatalytics.pages.dev |
| Branch Preview URL: | https://fix-upload-valid.geodatalytics.pages.dev |
995f53a to
ff2220a
Compare
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
relatively minor question about using explict true vs not a string check on rule validation.
This kind of brings up something else, would this be cleaner to utilize a v-form with a v-model="isFormValid" and use that variable to disable the submit button and || it with submitting. Then it checks all of the rules in the v-form automatically without needing the computed function combining previous rules?
https://vuetifyjs.com/en/components/forms/#usage
https://vuetifyjs.com/en/components/forms/#validation-state
I would understand not wanting to make a large a change to to this component.
| l.name && | ||
| l.files.length && | ||
| fileUploadRules.every((rule) => rule(l.files) === undefined) && | ||
| fileUploadRules.every((rule) => typeof rule(l.files) !== "string") && |
There was a problem hiding this comment.
Sorry again this is minor. I know that your current fileUploadRules is a single rule that returns and error string or a true value.
Should the check be more inline with vuetify to check for explict true (=== true) instead of if the typeof rule(l.files) !== "string"?
https://vuetifyjs.com/en/api/v-validation/#props-rules - the documentation supports true, false, string. So in the future if you have more rules and they return 'false (meaning not valid) they could pass this check.
I do understand that this is inside of a computed function that will ultimately evaluate to either true/false and that it isn't attached to vuetify's :rule directive but wondering if the change would help for consistency.
There was a problem hiding this comment.
Yeah I agree that === true is more consistent and readable. Changed in 9542e69
ff2220a to
9542e69
Compare
|
It's true that a The frame method and frame property are controlled with |
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Thanks for the explanation about the vbtn-toggle and the interaction with the frame stuff. I didn't think of that when I quickly looked at the component. Everything looks good.
2 participants
While writing the documentation for dataset upload, I found a bug in the validation logic. The max upload size validation rule never returns
undefined; it returns a string error message ortrue. In the definition of the computedvalidvariable, we had checked forundefinedvalues. This PR changes that logic to check whether an error message string was returned.